stats/opentelemetry: record retry attempts from clientStream #8342

vinothkumarr227 · 2025-05-19T09:14:38Z

Fixes: #8299

RELEASE NOTES:

stats/opentelemetry: Retry attempts (grpc.previous-rpc-attempts) are now recorded as span attributes for non-transparent client retries.

codecov · 2025-05-19T09:19:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.20%. Comparing base (f2d3e11) to head (11523b6).
Report is 13 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8342      +/-   ##
==========================================
- Coverage   82.41%   82.20%   -0.22%     
==========================================
  Files         419      419              
  Lines       42025    42065      +40     
==========================================
- Hits        34637    34580      -57     
- Misses       5944     6013      +69     
- Partials     1444     1472      +28

Files with missing lines	Coverage Δ
stats/opentelemetry/client_metrics.go	`85.16% <100.00%> (+0.08%)`	⬆️
stats/opentelemetry/client_tracing.go	`88.57% <100.00%> (+1.07%)`	⬆️
stats/opentelemetry/opentelemetry.go	`77.90% <ø> (ø)`
stats/opentelemetry/server_tracing.go	`74.19% <100.00%> (+0.86%)`	⬆️
stats/opentelemetry/trace.go	`89.83% <100.00%> (+0.94%)`	⬆️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

purnesh42H · 2025-05-19T09:37:52Z

stream.go

@@ -430,6 +432,7 @@ func (cs *clientStream) newAttemptLocked(isTransparent bool) (*csAttempt, error)
 			IsServerStream:            cs.desc.ServerStreams,
 			IsTransparentRetryAttempt: isTransparent,
 		}
+		ctx = context.WithValue(ctx, clientStreamKey{}, cs.numRetries)


as @dfawley suggested, we should just compute the retry attempts within otel interceptors #8299 (comment). While incrementing in trace.go, we should just skip if transparent retry is true. Re-using cs.numRetries is not advisable.

Thanks! Updated to compute retry attempts in the otel interceptor (TagRPC) and no longer using cs.numRetries

purnesh42H · 2025-05-20T09:48:07Z

stats/opentelemetry/trace.go

 		// Note: Go always added Client and FailFast attributes even though they are not
 		// defined by the OpenCensus gRPC spec. Thus, they are unimportant for
 		// correctness.
 		span.SetAttributes(
 			attribute.Bool("Client", rs.Client),
 			attribute.Bool("FailFast", rs.FailFast),
 			attribute.Int64("previous-rpc-attempts", int64(ai.previousRPCAttempts)),
+			attribute.Int64("retry-attempts", int64(atomic.LoadUint32(&ai.explicitRetryCount))),


we shouldn't be adding a new attribute. previous-rpc-attempts should be enough to depict retries. If > 0, that means there were retries. You should be doing the increment here if transparent-retry is false.

See https://github.com/grpc/proposal/blob/master/A72-open-telemetry-tracing.md#tracing-information. We should be confined to attributes defined here.

purnesh42H · 2025-05-20T10:01:05Z

stats/opentelemetry/client_tracing.go

@@ -121,6 +122,10 @@ func (h *clientTracingHandler) HandleConn(context.Context, stats.ConnStats) {}
 // TagRPC implements per RPC attempt context management for traces.
 func (h *clientTracingHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context {
 	ctx, ai := getOrCreateRPCAttemptInfo(ctx)
+	if ai.previousRPCAttempts > 0 {
+		atomic.AddUint32(&ai.explicitRetryCount, 1)


Retry attempts need to be counted by storing data in the context in a per-call interceptor. If you store it in the attemptInfo, it will get reset every attempt.

purnesh42H · 2025-05-20T10:02:16Z

stats/opentelemetry/client_tracing.go

+	if ai.previousRPCAttempts > 0 {
+		atomic.AddUint32(&ai.explicitRetryCount, 1)
+	}
+	atomic.AddUint32(&ai.previousRPCAttempts, 1)


you can maintain single attribute. Either previousRPCAttempts or explicitRetryCount. Why do we need both?

Thanks. I've removed explicitRetryCount and now use only previousRPCAttempts to track retries.

purnesh42H

I remember we had separate tests for retries. This change should only affect that test. This change shouldn't affect tests which are doing only single attempt.

purnesh42H · 2025-05-21T06:17:39Z

stats/opentelemetry/client_tracing.go

 // TagRPC implements per RPC attempt context management for traces.
 func (h *clientTracingHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context {
 	ctx, ai := getOrCreateRPCAttemptInfo(ctx)
+	var counter *int32
+	if val := ctx.Value(retryCountKey{}); val != nil {
+		counter = val.(*int32)


this again seems wrong. We should create the retry count key in the interceptor call which is per RPC. TagRPC is for per attempt. Here we should only retrieve the current retry count value (if needed)

Thanks for the clarification. I've updated the code accordingly — the retry count key is now created within the interceptor to ensure it's per-RPC

purnesh42H · 2025-05-21T06:19:11Z

stats/opentelemetry/trace.go

+		if !rs.IsTransparentRetryAttempt {
+			if val := ai.ctx.Value(retryCountKey{}); val != nil {
+				// Atomic increment and get new value
+				retryCount = uint32(atomic.AddInt32(val.(*int32), 1))


this value needs to be set back to context somewhere? otherwise we have lost the count.

purnesh42H · 2025-05-21T06:24:35Z

stats/opentelemetry/trace.go

 		// Note: Go always added Client and FailFast attributes even though they are not
 		// defined by the OpenCensus gRPC spec. Thus, they are unimportant for
 		// correctness.
 		span.SetAttributes(
 			attribute.Bool("Client", rs.Client),
 			attribute.Bool("FailFast", rs.FailFast),
-			attribute.Int64("previous-rpc-attempts", int64(ai.previousRPCAttempts)),
+			attribute.Int64("previous-rpc-attempts", int64(retryCount)),


Please note as per https://github.com/grpc/proposal/blob/master/A72-open-telemetry-tracing.md#tracing-information, previousRPCAttempts is the count of retries before current, so we should set here the existing retryCount value (before incrementing it). However, we should increment retry count after and set back to context for future.

purnesh42H · 2025-05-21T06:26:08Z

stats/opentelemetry/e2e_test.go

@@ -928,7 +928,7 @@ func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {
 				},
 				{
 					Key:   "previous-rpc-attempts",
-					Value: attribute.IntValue(0),
+					Value: attribute.IntValue(1),


It should be correctly 0 because it counts previous rpc attempts which means before current so for the first attempt it should be 0.

purnesh42H

@vinothkumarr227 have you tested this to ensure its working correctly? I was under impression that TestTraceSpan_WithRetriesAndNameResolutionDelay will need changes for expected values. I think its not working as expected because you are not setting the count back to ctx after incrementing.

purnesh42H · 2025-05-26T04:15:10Z

stats/opentelemetry/client_tracing.go

@@ -48,6 +48,8 @@ func (h *clientTracingHandler) initializeTraces() {
 }

 func (h *clientTracingHandler) unaryInterceptor(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
+	ci := &callInfo{numRetries: 0}


both these operations can be done in getOrCreateCallInfo? That we set call info in ctx only once

purnesh42H · 2025-05-26T04:15:18Z

stats/opentelemetry/client_tracing.go

@@ -58,6 +60,8 @@ func (h *clientTracingHandler) unaryInterceptor(ctx context.Context, method stri
 }

 func (h *clientTracingHandler) streamInterceptor(ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, streamer grpc.Streamer, opts ...grpc.CallOption) (grpc.ClientStream, error) {
+	ci := &callInfo{numRetries: 0}


same here about moving both to getOrCreateCallInfo

purnesh42H · 2025-05-26T04:17:11Z

stats/opentelemetry/opentelemetry.go

@@ -179,6 +179,8 @@ type callInfo struct {
 	// nameResolutionEventAdded is set when the resolver delay trace event
 	// is added. Prevents duplicate events, since it is reported per-attempt.
 	nameResolutionEventAdded atomic.Bool
+	// numRetries holds the count of non-transparent retry attempts.
+	numRetries int32


better to declare it as atomic and do regular operation for it otherwise you need to explicitly mention that this field need to be accessed atomically. Former is better IMO.

purnesh42H · 2025-05-26T04:22:29Z

stats/opentelemetry/trace.go

+		// retry.
+		if !rs.IsTransparentRetryAttempt {
+			if ci, ok := getRetryCount(ai.ctx); ok {
+				atomic.AddInt32(&ci.numRetries, 1)


As called out before, we need to set this back to context.

Also, it might be more clear if we name this field as well to previousRPCAttempts because at the time of retrieving its not accounting the current retry.

Thanks for the clarification. I've updated the code accordingly

purnesh42H · 2025-05-26T04:23:23Z

stats/opentelemetry/opentelemetry.go

+	return context.WithValue(ctx, retryCountKey{}, ci)
+}
+
+// getRetryCount retrieves the retry count tracking struct from the context.


no need of docstring since the function is simple enough to understand.

purnesh42H · 2025-05-26T04:25:07Z

stats/opentelemetry/opentelemetry.go

+}
+
+// getRetryCount retrieves the retry count tracking struct from the context.
+func getRetryCount(ctx context.Context) (*callInfo, bool) {


nit: its preferred to avoid get prefix in these situations https://g3doc.corp.google.com/go/g3doc/style/decisions.md?cl=head#getters

Thanks for the clarification. I've updated the code accordingly

purnesh42H · 2025-05-26T04:26:19Z

stats/opentelemetry/e2e_test.go

@@ -1708,6 +1710,32 @@ func verifyTrace(t *testing.T, spans tracetest.SpanStubs, want traceSpanInfo) {
 	}
 }

+func verifyPreviousRPCAttempts(t *testing.T, spans tracetest.SpanStubs) {


why this needs to be a separate function? We are just verifying the previous-rpc-attempts which is already being compared. We shouldn't need this.

purnesh42H · 2025-05-28T04:36:08Z

stats/opentelemetry/client_metrics.go

@@ -81,6 +81,7 @@ func getOrCreateCallInfo(ctx context.Context, cc *grpc.ClientConn, method string
 		}
 		ctx = setCallInfo(ctx, ci)
 	}
+	ctx = setRetryCount(ctx, ci)


Its weird that we are maintaining the ci instance in 2 separate keys. We should either just hold the retry count in the retry count key or keep one key value pair of callInfo with field tracking retryCount. Either way its fine but since you have already added retryCount to callInfo, we should only setCallInfo and retryCount should just be another field in that.

Also, let's rename the field previousRPCAttempts since that's what we are updating it to.

ci = &callInfo{ target: cc.CanonicalTarget(), method: determineMethod(method, opts...), previousRPCAttempts: 0 } ctx = setCallInfo(ctx, ci)

Then you can remove line 84

purnesh42H · 2025-05-28T04:40:33Z

stats/opentelemetry/e2e_test.go

-				observedSpans[val] = append(observedSpans[val], span.Name)
+		for _, wantAttr := range want.attributes {
+			for _, attr := range span.Attributes {
+				fmt.Println("Span Name", span.Name)


why we need these print statements?

purnesh42H · 2025-05-28T04:47:07Z

stats/opentelemetry/opentelemetry.go

@@ -179,8 +179,8 @@ type callInfo struct {
 	// nameResolutionEventAdded is set when the resolver delay trace event
 	// is added. Prevents duplicate events, since it is reported per-attempt.
 	nameResolutionEventAdded atomic.Bool
-	// numRetries holds the count of non-transparent retry attempts.
-	numRetries int32
+	// previousRPCAttempts holds the count of non-transparent retry attempts.


// previousRPCAttempts holds the count of RPC attempts that have happened before current attempt. Transparent // retries are excluded.

purnesh42H · 2025-05-28T04:47:47Z

stats/opentelemetry/opentelemetry.go

@@ -219,8 +219,7 @@ func setRetryCount(ctx context.Context, ci *callInfo) context.Context {
 	return context.WithValue(ctx, retryCountKey{}, ci)
 }

-// getRetryCount retrieves the retry count tracking struct from the context.
-func getRetryCount(ctx context.Context) (*callInfo, bool) {
+func retryCount(ctx context.Context) (*callInfo, bool) {


we won't need it anymore as we already have for callInfo

purnesh42H · 2025-05-28T04:48:04Z

stats/opentelemetry/trace.go

-				atomic.AddInt32(&ci.numRetries, 1)
+			if ci, ok := retryCount(ai.ctx); ok {
+				ci.previousRPCAttempts.Add(1)
+				ai.ctx = setRetryCount(ai.ctx, ci)


now, we will call setCallInfo here

purnesh42H · 2025-05-28T04:48:34Z

stats/opentelemetry/client_tracing.go

@@ -118,9 +118,15 @@ func (h *clientTracingHandler) TagConn(ctx context.Context, _ *stats.ConnTagInfo
 // HandleConn exists to satisfy stats.Handler for tracing.
 func (h *clientTracingHandler) HandleConn(context.Context, stats.ConnStats) {}

+type retryCountKey struct{}


won't be needed if we are maintaining within callInfo

purnesh42H · 2025-05-28T04:48:51Z

stats/opentelemetry/client_tracing.go

 // TagRPC implements per RPC attempt context management for traces.
 func (h *clientTracingHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context {
 	ctx, ai := getOrCreateRPCAttemptInfo(ctx)
+	if ci, ok := retryCount(ctx); ok {


this will call getOrCreateCallInfo

purnesh42H · 2025-05-28T04:49:32Z

stats/opentelemetry/client_tracing.go

+	if ci, ok := retryCount(ctx); ok {
+		ai.previousRPCAttempts = uint32(ci.previousRPCAttempts.Load())
+	}
+	ai.ctx = ctx


why set context here? This will happen later anyway?

We need to explicitly set ai.ctx = ctx here because populateSpan later uses ai.ctx to retrieve and update callInfo. If we skip this assignment, ai.ctx remains nil, leading to a nil pointer.

purnesh42H · 2025-05-28T04:54:01Z

stats/opentelemetry/e2e_test.go

-			if diff := cmp.Diff(want.events, span.Events, cmpopts.IgnoreFields(trace.Event{}, "Time")); diff != "" {
-				t.Errorf("Span event mismatch for %q (kind: %s) (-want +got):\n%s",
-					want.name, want.spanKind, diff)
+			if len(want.events) > 0 {


why do we have to make any change to this method? If we declare the wantSpanInfos in the caller test like here https://github.com/grpc/grpc-go/blob/master/stats/opentelemetry/e2e_test.go#L1082-L1240 we shouldn't need to change anything here.

I’ve added new test cases for TestRetrySpans_UnaryCallAttributes

purnesh42H · 2025-05-28T04:55:44Z

stats/opentelemetry/e2e_test.go

@@ -1677,31 +1678,58 @@ func (s) TestTraceSpan_WithRetriesAndNameResolutionDelay(t *testing.T) {
 				t.Fatalf("%s call failed: %v", tt.name, err)
 			}

-			wantSpanInfo := traceSpanInfo{
+			methodName := strings.TrimPrefix(tt.spanName, "Sent.")


this is not very easy to understand. For tests its usually better to have static declarations for expected values instead of forming at runtime. Can we do similar to how its done in other tests https://github.com/grpc/grpc-go/blob/master/stats/opentelemetry/e2e_test.go#L1082-L1240. Then you don't need to make changes in verifyTrace

if we use static declarations for expected values, we encounter issues when the LB policy provides a new pick value (reference). That’s why we’re using verifyTrace and focusing the test specifically on the name resolution delay event. For that reason, I’ve tested both the previous-rpc-attempts and the name-resolution events.

I’ve added new test cases for TestRetrySpans_UnaryCallAttributes

Fixed retry attempts in HandleRPC

3ba457e

purnesh42H reviewed May 19, 2025

View reviewed changes

purnesh42H assigned vinothkumarr227 May 19, 2025

purnesh42H added Type: Bug Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability labels May 19, 2025

purnesh42H added this to the 1.73 Release milestone May 19, 2025

vinothkumarr227 added 2 commits May 20, 2025 06:55

Fixed the review changes

5d19779

Fixed vet issues

4245950

vinothkumarr227 requested a review from purnesh42H May 20, 2025 07:09

eshitachandwani assigned purnesh42H and unassigned vinothkumarr227 May 20, 2025

purnesh42H reviewed May 20, 2025

View reviewed changes

purnesh42H assigned vinothkumarr227 and unassigned purnesh42H May 20, 2025

Fixed the review changes

5347db1

vinothkumarr227 requested a review from purnesh42H May 21, 2025 06:08

purnesh42H reviewed May 21, 2025

View reviewed changes

Fixed the review changes

99e88d8

vinothkumarr227 requested a review from purnesh42H May 22, 2025 07:51

purnesh42H reviewed May 26, 2025

View reviewed changes

vinothkumarr227 added 2 commits May 26, 2025 12:58

Fixed the review changes

586cf63

Fixed the test cases

1bdad7e

eshitachandwani assigned purnesh42H and unassigned vinothkumarr227 May 27, 2025

eshitachandwani requested a review from purnesh42H May 27, 2025 09:37

purnesh42H reviewed May 28, 2025

View reviewed changes

purnesh42H assigned vinothkumarr227 and unassigned purnesh42H May 28, 2025

Fixed the review changes

39c5f0d

small tweaks

11523b6

stats/opentelemetry: record retry attempts from clientStream #8342

Are you sure you want to change the base?

stats/opentelemetry: record retry attempts from clientStream #8342

Uh oh!

Conversation

vinothkumarr227 commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purnesh42H left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purnesh42H left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinothkumarr227 commented May 19, 2025 •

edited

Loading

codecov bot commented May 19, 2025 •

edited

Loading